backend/init_data/skills/dingtalk-docs/provider.py (1)

672-685: Centralize filename generation instead of duplicating it here.

This sanitizer now exists in both backend/init_data/skills/dingtalk-docs/provider.py and backend/app/services/dingtalk/docs_service.py. Keeping two copies will drift the next time the naming rules change.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/init_data/skills/dingtalk-docs/provider.py` around lines 672 - 685,
Duplicate filename-sanitization logic exists in _build_filename (class in
provider.py) and in docs_service; extract the logic into a single shared
function (e.g., sanitize_doc_filename(title: str, modified_time: str) or
build_doc_filename(title, modified_time)) in a common module and have
_build_filename call that function instead of reimplementing the regex/untitled
fallback; update docs_service to import and use the same shared function so both
places reference one implementation and remove the duplicated
regex/strip/untitled code from _build_filename.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

backend/app/services/dingtalk/docs_service.py (2)

181-200: ⚠️ Potential issue | 🟠 Major

Placeholder implementation returns synthetic metadata.

get_document_info() fabricates title and modified_time from the URL and local clock instead of calling the DingTalk API. This means downstream filenames and KB records will contain synthetic data rather than actual document metadata.

This has been flagged in a previous review. Ensure this is addressed before the feature is production-ready.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/app/services/dingtalk/docs_service.py` around lines 181 - 200, The
current get_document_info function returns synthetic metadata (title,
modified_time, modified_time_formatted) built from doc_id and local clock;
replace this placeholder by calling the DingTalk document metadata API to fetch
real values (title, lastModified, content type, url) and map them into the
returned dict (fields title, modified_time, modified_time_formatted,
content_type, url, doc_id); preserve a safe fallback path that logs the API
error and only then uses the synthetic values so downstream code (file naming
and KB records) receives real metadata when available and predictable fallback
data when the API fails.

---

229-247: ⚠️ Potential issue | 🟠 Major

Placeholder implementation returns dummy content.

download_document_content() returns hardcoded placeholder markdown instead of fetching actual document content from DingTalk. Any KB entry created through this path will contain fake content.

This has been flagged in a previous review. Ensure this is addressed before the feature is production-ready.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/app/services/dingtalk/docs_service.py` around lines 229 - 247,
download_document_content currently returns hardcoded markdown; replace the
placeholder logic in download_document_content to call DingTalk's document
export/download APIs using the doc_info and export_format, download the exported
content (handling possible export conversion for "markdown" vs other formats),
parse/validate the response and populate "content", "title", "modified_time",
"modified_time_formatted", "file_extension" and "doc_id" from the real API
response, and add robust error handling and retries (log and raise on
unrecoverable failures) so KB entries contain the actual document content rather
than dummy text.

backend/init_data/skills/dingtalk-docs/provider.py (2)

408-452: ⚠️ Potential issue | 🟠 Major

Placeholder: _get_document_info_from_mcp does not call MCP or DingTalk.

This method extracts the document ID from the URL and fabricates a synthetic title/timestamp rather than fetching real metadata. The workflow cannot retrieve actual document information.

This has been flagged in a previous review.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/init_data/skills/dingtalk-docs/provider.py` around lines 408 - 452,
The _get_document_info_from_mcp function currently only parses doc_id from
doc_url and synthesizes metadata; replace the placeholder logic with a real
MCP/DingTalk metadata fetch using the provided sandbox (use the sandbox/tool
client or the MCP helper available in this environment) to call the appropriate
MCP endpoint with the extracted doc_id (or the full doc_url) and return the real
fields: success (bool), doc_id, title, modified_time (ISO),
modified_time_formatted; preserve the existing error handling pattern (return
{"success": False, "error": ...}) and ensure you fall back to an informative
error when the MCP call fails or returns no data so callers of
_get_document_info_from_mcp get real metadata instead of synthetic values.

---

454-497: ⚠️ Potential issue | 🟠 Major

Placeholder: _download_document_content returns template content.

This method generates placeholder markdown instead of downloading the actual DingTalk document body. KB entries created through this path will contain dummy text.

This has been flagged in a previous review.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/init_data/skills/dingtalk-docs/provider.py` around lines 454 - 497,
_download_document_content currently returns placeholder markdown instead of
fetching the real document body; replace the template generation with an actual
fetch from DingTalk via the MCP/client used elsewhere (use the same client or
helper that performs authenticated requests), extract the document ID from
doc_url (doc_id) using the existing patterns, call the appropriate MCP/DingTalk
API endpoint to retrieve the document content (handle JSON/html responses),
convert/clean the response into the expected markdown/plain string, and return
{"success": True, "content": content}; ensure robust error handling around the
network call (catch and log exceptions and return {"success": False, "error":
str(e)}), and reuse existing symbols/functions for HTTP auth/client if present
to avoid duplicating credentials.

backend/app/mcp_server/tools/dingtalk_docs.py (2)

239-246: Consider making this handler async for consistency.

add_dingtalk_doc_with_attachment is defined as a synchronous function (def) while the other two tools are async def. If FastMCP handles both patterns correctly, this is fine, but async consistency would be cleaner and would allow using asyncio.to_thread() for the blocking orchestrator call.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/app/mcp_server/tools/dingtalk_docs.py` around lines 239 - 246, The
function add_dingtalk_doc_with_attachment is synchronous while similar handlers
are async; change its signature to async def
add_dingtalk_doc_with_attachment(...) and update its internals to run the
blocking orchestrator call via asyncio.to_thread (or await an async wrapper) so
the handler is non-blocking and consistent with the other async tools; ensure
any callers or tests that expect the old sync signature are updated to await
add_dingtalk_doc_with_attachment and preserve existing behavior for
trigger_indexing/trigger_summary and return type Dict[str, Any].

---

180-186: Move datetime import to module level.

The datetime import is performed twice inside the function at runtime. Since it's already used elsewhere in the codebase, move the import to the top of the file for consistency and minor performance improvement.

♻️ Proposed fix

Add to module-level imports:

 import logging
+from datetime import datetime
 from typing import Any, Dict, Optional

Then remove the inline imports at lines 180 and 184:

             logger.warning(
                 f"Invalid modified_time format: {modified_time}, using current time"
             )
-            from datetime import datetime
-
             modified_time = datetime.now().strftime("%Y%m%d%H%M%S")
     else:
-        from datetime import datetime
-
         modified_time = datetime.now().strftime("%Y%m%d%H%M%S")

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/app/mcp_server/tools/dingtalk_docs.py` around lines 180 - 186, Move
the repeated inline imports of datetime to the module level: add a single
module-level line "from datetime import datetime" to the top of the file, then
remove the two inline "from datetime import datetime" statements found near the
assignments to modified_time (the blocks that set modified_time =
datetime.now().strftime("%Y%m%d%H%M%S")). Ensure the existing uses of
modified_time still refer to the now-imported datetime and run tests/lint to
confirm no unused-import warnings.

backend/app/services/dingtalk/docs_service.py (2)

17-17: Unused import: httpx

The httpx module is imported but never used in the current implementation. This is likely intended for future DingTalk API calls.

♻️ Suggested fix

Either remove the unused import until needed:

-import httpx
-
 from app.core.config import settings

Or add a comment indicating it's reserved for future use:

import httpx  # Reserved for DingTalk API calls

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/app/services/dingtalk/docs_service.py` at line 17, The import httpx
at module-level in docs_service.py is unused; either remove the import statement
entirely or retain it with an explicit comment (e.g., "Reserved for DingTalk API
calls") to indicate intentional future use so linters don't flag it — update the
module-level import for httpx accordingly.

---

141-147: Log the parse failure before falling through.

The outer except Exception: pass swallows all errors silently. While the fallback to current time is acceptable, logging the exception would aid debugging parse failures.

♻️ Proposed fix

             except ValueError:
                 continue
-    except Exception:
-        pass
+    except Exception as e:
+        logger.warning(f"Unexpected error parsing modified_time '{modified_time}': {e}")
 
     # Return current time as fallback
     logger.warning(

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/app/services/dingtalk/docs_service.py` around lines 141 - 147, The
outer "except Exception: pass" in the block that tries to parse modified_time
silently swallows failures; update that outer handler to log the exception
(including exception details) and context (the modified_time value and attempted
fmt) before falling back to current time—modify the outer except around the
datetime.strptime/strftime loop to call the module/class logger (or existing
logger instance) with a descriptive message and the caught exception information
so parse failures are visible for debugging.

backend/app/services/dingtalk/__init__.py (1)

14-19: Sort __all__ alphabetically for consistency.

Static analysis flags that __all__ is not sorted. Per isort standards, sorting helps maintainability.

♻️ Proposed fix

 __all__ = [
+    "build_dingtalk_doc_filename",
     "DingTalkDocsService",
     "dingtalk_docs_service",
     "sanitize_filename",
-    "build_dingtalk_doc_filename",
 ]

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/app/services/dingtalk/__init__.py` around lines 14 - 19, The __all__
list in this module is not alphabetically sorted; reorder the entries in __all__
so they are lexicographically sorted (e.g., "DingTalkDocsService" before
"build_dingtalk_doc_filename", "dingtalk_docs_service", and "sanitize_filename")
to satisfy static analysis/isort expectations and maintain consistency.

backend/init_data/skills/dingtalk-docs/provider.py (3)

75-77: Use raise ... from None to clarify exception origin.

Per B904, when re-raising in an except block, use raise ... from None to indicate the exception is intentional and not from mishandled exception processing.

♻️ Proposed fix

     else:
-        raise ImportError(
+        raise ImportError(
             "Cannot import BaseSandboxTool from chat_shell.tools.sandbox._base"
-        )
+        ) from None

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/init_data/skills/dingtalk-docs/provider.py` around lines 75 - 77, The
ImportError being raised for failing to import BaseSandboxTool from
chat_shell.tools.sandbox._base should be re-raised using exception chaining
suppression; modify the raise in the provider module where BaseSandboxTool is
checked so it uses "raise ImportError(... ) from None" to indicate the error is
intentional and not caused by a prior exception.

---

461-462: Remove redundant import re inside method.

Same issue - re is already imported at module level.

♻️ Proposed fix

         try:
             # Extract doc ID for content generation
-            import re
-
             doc_id = None

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/init_data/skills/dingtalk-docs/provider.py` around lines 461 - 462,
Remove the redundant local import of the regex module inside the method (the
`import re` line shown near the "Extract doc ID for content generation" block)
since `re` is already imported at module level; simply delete that inner import
so the method uses the module-level `re` symbol (locate the local import inside
the provider.py function that extracts the doc ID and remove it).

---

415-416: Remove redundant import re inside method.

re is already imported at the module level (line 20). The local import is unnecessary.

♻️ Proposed fix

         try:
             # Extract doc ID from URL
-            import re
-
             doc_id = None

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/init_data/skills/dingtalk-docs/provider.py` around lines 415 - 416,
Remove the redundant local "import re" inside the method that extracts the doc
ID from a URL and rely on the module-level "re" import instead; delete the
inline import statement where the comment "Extract doc ID from URL" appears (the
duplicate import inside that function) so the function uses the already-imported
regex module, then run linters/tests to confirm no unused-import warnings
remain.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

backend/app/mcp_server/tools/dingtalk_docs.py (1)

140-143: ⚠️ Potential issue | 🟠 Major

Keep the synchronous document-creation phase off the async handler.

add_dingtalk_doc_to_knowledge() is async, but it creates a synchronous SessionLocal() and then calls knowledge_orchestrator.create_document_with_content() directly. That blocks the FastMCP event loop, and the obvious asyncio.to_thread(..., db=db) fix would also be unsafe because the current SQLAlchemy session is not thread-safe. Move the whole DB/orchestrator phase behind a worker-thread helper that opens and closes its own SessionLocal().

Also applies to: 205-214

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/app/mcp_server/tools/dingtalk_docs.py` around lines 140 - 143, The
async handler add_dingtalk_doc_to_knowledge is creating a synchronous
SessionLocal() and calling knowledge_orchestrator.create_document_with_content
directly which blocks the event loop; refactor by extracting the DB/orchestrator
work (including opening/closing a fresh SessionLocal and calling
_get_user_from_token and knowledge_orchestrator.create_document_with_content)
into a synchronous helper function (e.g.,
sync_create_dingtalk_doc(db_work_helper) that creates its own SessionLocal,
performs the user lookup and document creation, closes the session, and returns
results) and then call that helper from the async function via
asyncio.to_thread(sync_create_dingtalk_doc, ...), ensuring no existing
SQLAlchemy SessionLocal instance is shared across threads. Ensure the same
change is applied to the other occurrence around lines 205-214.

backend/app/services/dingtalk/docs_service.py (2)

261-309: ⚠️ Potential issue | 🔴 Critical

Do not fabricate metadata on MCP miss or failure.

The missing-config path, the catch-all error path, and even the Line 286/Line 287 defaults all manufacture a title/timestamp and return them as if DingTalk had confirmed them. That makes get_document_info() report success with fake metadata and reintroduces the earlier bad-filename/bad-record failure mode.

🧭 Patch sketch

         mcp_config = self._get_dingtalk_docs_mcp_config(user_preferences)
         if not mcp_config:
-            logger.warning(
-                "dingtalk_docs MCP not configured, returning placeholder info"
-            )
-            # Fallback to placeholder if MCP not configured
-            now = datetime.now()
-            return {
-                "doc_id": doc_id,
-                "title": f"DingTalkDoc_{doc_id[:8]}",
-                "modified_time": now.isoformat(),
-                "modified_time_formatted": now.strftime("%Y%m%d%H%M%S"),
-                "content_type": "markdown",
-                "url": doc_url,
-            }
+            raise ValueError(
+                "dingtalk_docs MCP not configured. "
+                "Please configure DingTalk Docs MCP in user settings."
+            )
@@
-            title = result.get("title", f"DingTalkDoc_{doc_id[:8]}")
-            modified_time = result.get("modified_time", datetime.now().isoformat())
+            title = result.get("title")
+            modified_time = result.get("modified_time")
+            if not title or not modified_time:
+                raise ValueError(
+                    "MCP response missing required document metadata"
+                )
@@
-        except Exception as e:
-            logger.error(f"Failed to get document info from MCP: {e}")
-            # Fallback to placeholder on error
-            now = datetime.now()
-            return {
-                "doc_id": doc_id,
-                "title": f"DingTalkDoc_{doc_id[:8]}",
-                "modified_time": now.isoformat(),
-                "modified_time_formatted": now.strftime("%Y%m%d%H%M%S"),
-                "content_type": "markdown",
-                "url": doc_url,
-            }
+        except Exception as e:
+            logger.error(f"Failed to get document info from MCP: {e}")
+            raise ValueError(f"Failed to get document info: {e}") from e

---

95-113: ⚠️ Potential issue | 🟠 Major

Validate the hostname before extracting a document ID.

re.search() scans the full input string, so an unrelated URL or arbitrary text that merely embeds alidocs.dingtalk.com/i/... in a query or fragment will pass validation. Because the original doc_url is then forwarded unchanged to the downstream MCP call, this boundary currently accepts non-DingTalk URLs instead of rejecting them up front.

🔒 Patch sketch

+from urllib.parse import urlparse
+
 def _extract_doc_id_from_url(self, url: str) -> Optional[str]:
     if not url:
         return None
+    parsed = urlparse(url)
+    if parsed.scheme not in {"http", "https"}:
+        return None
+    if parsed.hostname != "alidocs.dingtalk.com":
+        return None
+    path = parsed.path.rstrip("/")

-    node_pattern = r"alidocs\.dingtalk\.com/i/nodes/([a-zA-Z0-9_-]+)"
-    match = re.search(node_pattern, url)
+    match = re.fullmatch(r"/i/nodes/([a-zA-Z0-9_-]+)", path)
     if match:
         return match.group(1)

-    docs_pattern = r"alidocs\.dingtalk\.com/i/team/[^/]+/docs/([a-zA-Z0-9_-]+)"
-    match = re.search(docs_pattern, url)
+    match = re.fullmatch(r"/i/team/[^/]+/docs/([a-zA-Z0-9_-]+)", path)
     if match:
         return match.group(1)

-    wiki_pattern = r"alidocs\.dingtalk\.com/i/team/[^/]+/wiki/([a-zA-Z0-9_-]+)"
-    match = re.search(wiki_pattern, url)
+    match = re.fullmatch(r"/i/team/[^/]+/wiki/([a-zA-Z0-9_-]+)", path)
     if match:
         return match.group(1)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/app/services/dingtalk/docs_service.py` around lines 95 - 113, The
extraction currently uses re.search over the whole url string (node_pattern,
docs_pattern, wiki_pattern) which can match embedded text in unrelated URLs;
instead parse the url with urllib.parse.urlparse, validate that the parsed
hostname (netloc) is exactly or endswith "alidocs.dingtalk.com" (depending on
allowed subdomains), and then run your regexes against only the parsed path (or
use path-based pattern matching) to extract and return the document id; update
the code that references url, node_pattern, docs_pattern, and wiki_pattern to
perform the hostname check before returning any match and return None if the
hostname is not the expected DingTalk host.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

backend/init_data/skills/dingtalk-docs/provider.py (3)

861-905: Consider simplifying the redundant cleanup logic.

The cleanup code at lines 886-893 runs inside the main try block (after sandbox.commands.run), but the same cleanup also exists in the except block (lines 897-904). This works but is redundant since a single finally block would handle both paths.

♻️ Simplified structure

             try:
                 # Build curl command using array style to prevent shell injection
                 import shlex

                 curl_cmd = [
                     "curl",
                     "-s",
                     "-X",
                     "POST",
                     "-H",
                     f"Authorization: Bearer {auth_token}",
                     "-H",
                     "Content-Type: application/json",
                     "-d",
                     f"@{payload_file}",
                     mcp_url,
                 ]

                 result_obj = await sandbox.commands.run(
                     cmd=shlex.join(curl_cmd),
                     cwd="/home/user",
                     timeout=60,
                 )
-
-                # Clean up temp file after use
-                try:
-                    await sandbox.commands.run(
-                        cmd=f"rm -f {payload_file}",
-                        cwd="/home/user",
-                        timeout=10,
-                    )
-                except Exception:
-                    pass
-
-            except Exception as e:
-                # Clean up temp file on error
+            finally:
                 try:
                     await sandbox.commands.run(
                         cmd=f"rm -f {payload_file}",
                         cwd="/home/user",
                         timeout=10,
                     )
                 except Exception:
                     pass
-                raise e

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/init_data/skills/dingtalk-docs/provider.py` around lines 861 - 905,
The temp-file cleanup for payload_file is duplicated in both the success path
and exception path; refactor the try/except to a try/finally around the
sandbox.commands.run invocation that executes the rm -f {payload_file} cleanup
(called via sandbox.commands.run) in the finally block, remove the duplicate
cleanup in the except branch, and keep error re-raising behavior for exceptions
from sandbox.commands.run; use the existing curl_cmd/shlex.join, auth_token and
mcp_url variables as before to locate the code to change.

---

74-77: Use raise ... from None to distinguish from exception handling errors.

When re-raising an ImportError within an except clause, use from None to suppress the exception chain and clarify intent.

♻️ Proposed fix

     else:
-        raise ImportError(
+        raise ImportError(
             "Cannot import BaseSandboxTool from chat_shell.tools.sandbox._base"
-        )
+        ) from None

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/init_data/skills/dingtalk-docs/provider.py` around lines 74 - 77, The
ImportError raised in the else branch that references "Cannot import
BaseSandboxTool from chat_shell.tools.sandbox._base" should be suppressed from
chaining by using "raise ... from None"; locate the else block in provider.py
where the ImportError is raised (the message about BaseSandboxTool and
chat_shell.tools.sandbox._base) and modify the raise to use "from None" so the
exception chain is not preserved.

---

548-592: Remove dead placeholder code.

This _download_document_content method returns placeholder content and is never called—the workflow at line 303 uses _download_document_content_real instead. Remove this dead code to avoid confusion.

🗑️ Remove dead code

-    async def _download_document_content(self, sandbox: Any, doc_url: str) -> dict:
-        """Download document content from DingTalk.
-
-        For now, this returns placeholder content.
-        In production, this would call the DingTalk API via MCP.
-        """
-        try:
-            # Extract doc ID for content generation
-            import re
-
-            doc_id = None
-            patterns = [
-                r"alidocs\.dingtalk\.com/i/nodes/([a-zA-Z0-9_-]+)",
-                r"alidocs\.dingtalk\.com/i/team/[^/]+/docs/([a-zA-Z0-9_-]+)",
-            ]
-
-            for pattern in patterns:
-                match = re.search(pattern, doc_url)
-                if match:
-                    doc_id = match.group(1)
-                    break
-
-            # Generate placeholder content
-            content = f"""# DingTalk Document
-
-This document was imported from DingTalk.
-
-**Source URL:** {doc_url}
-**Document ID:** {doc_id or "unknown"}
-**Imported at:** {datetime.now().isoformat()}
-
-## Content
-
-The actual content would be fetched from DingTalk API in production.
-This is a placeholder for the document content.
-
----
-*Imported by Wegent DingTalk Docs Skill*
-"""
-
-            return {"success": True, "content": content}
-
-        except Exception as e:
-            return {"success": False, "error": str(e)}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/init_data/skills/dingtalk-docs/provider.py` around lines 548 - 592,
Remove the dead placeholder method _download_document_content (its docstring,
body and exception handling) from the DingTalk docs provider since the workflow
uses _download_document_content_real; also remove any now-unused imports
referenced only by that method (e.g., re, datetime) and ensure there are no
remaining references to _download_document_content elsewhere in the module.

backend/app/services/dingtalk/docs_service.py (2)

191-225: Extract repeated numeric literals into named constants.

The literals at Line 209 (60.0), Line 219/413 (401, 403), and Line 392 (doc_id[:8]) should be constants for maintainability.

As per coding guidelines, "Extract magic numbers to named constants in Python code."

Also applies to: 392-405

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/app/services/dingtalk/docs_service.py` around lines 191 - 225,
Replace magic numeric literals with descriptive constants: define
DEFAULT_HTTP_TIMEOUT = 60.0 and use it when creating
httpx.AsyncClient(timeout=DEFAULT_HTTP_TIMEOUT); define AUTH_ERROR_CODES = (401,
403) and use it in the HTTPStatusError handling instead of hard-coded 401/403;
define DOC_ID_PREVIEW_LEN = 8 and use it wherever doc_id[:8] is used. Update the
references in the MCP call function (the payload/call block and the except
httpx.HTTPStatusError handler) and in the code that slices doc_id so all
occurrences use these named constants (e.g., in the function that constructs doc
previews or logs). Ensure constants are placed near the module top with
descriptive names and used throughout.

---

74-76: Drop the no-op constructor or add explicit return type annotation.

Line 74-76 defines an empty __init__ without -> None; either remove it or annotate it explicitly.

As per coding guidelines, "Python code MUST follow PEP 8, Black formatter (line length: 88), and isort standards with type hints required."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/app/services/dingtalk/docs_service.py` around lines 74 - 76, The
empty constructor def __init__(self): in docs_service.py is a no-op and missing
a return type; either remove this method entirely if it adds no behavior, or
annotate it with an explicit -> None return type (i.e., change to def
__init__(self) -> None:) to satisfy type-hinting rules and PEP8/Black
requirements; update the __init__ declaration accordingly in the class that
defines it.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

backend/app/services/dingtalk/docs_service.py (1)

507-529: ⚠️ Potential issue | 🟡 Minor

Remove unnecessary f-string prefixes and chain exceptions.

Lines 513-516 use f-strings without any placeholders. Additionally, the ValueError raises should chain the original exception with from e to preserve traceback context.

🔧 Proposed fix

         except httpx.HTTPStatusError as e:
             logger.error(
                 f"HTTP error from DingTalk MCP: {e.response.status_code} - {e.response.text}"
             )
             if e.response.status_code in AUTH_ERROR_CODES:
                 raise ValueError(
-                    f"DingTalk authentication required. Please ensure:\n"
-                    f"1. You have access to this document in DingTalk\n"
-                    f"2. The document is shared with you or is public\n"
-                    f"3. Your DingTalk MCP configuration is correct"
-                )
+                    "DingTalk authentication required. Please ensure:\n"
+                    "1. You have access to this document in DingTalk\n"
+                    "2. The document is shared with you or is public\n"
+                    "3. Your DingTalk MCP configuration is correct"
+                ) from e
             raise ValueError(
                 f"Failed to download document from DingTalk: HTTP {e.response.status_code}"
-            )
+            ) from e
         except Exception as e:
             logger.error(f"Failed to download document from MCP: {e}")
             error_msg = str(e)
             if "authentication" in error_msg.lower() or "auth" in error_msg.lower():
                 raise ValueError(
                     f"DingTalk authentication required: {error_msg}\n"
-                    f"Please ensure you have access to this document in DingTalk."
-                )
-            raise ValueError(f"Failed to download document: {e}")
+                    "Please ensure you have access to this document in DingTalk."
+                ) from e
+            raise ValueError(f"Failed to download document: {e}") from e

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/app/services/dingtalk/docs_service.py` around lines 507 - 529, In the
DingTalk docs service exception handlers (the except httpx.HTTPStatusError and
generic except Exception blocks in docs_service.py), remove the unnecessary
f-string prefixes from string literals (e.g., logger.error and ValueError
messages that have no placeholders) and change each bare "raise ValueError(...)"
to chain the original exception using "raise ValueError(... ) from e" so the
traceback/context is preserved; reference the logger, AUTH_ERROR_CODES, and the
caught exception variable e when making these edits.

backend/app/mcp_server/tools/dingtalk_docs.py (1)

195-217: ⚠️ Potential issue | 🟠 Major

The timestamped filename is not being used in document creation.

The filename is built at line 196 following the {title}_{timestamp}.md convention, but the document is created with name=title (line 212) instead of name=filename. The filename only appears in the response payload (line 223) but the actual document doesn't use it.

🔧 Proposed fix

         result = await asyncio.to_thread(
             knowledge_orchestrator.create_document_with_content,
             db=db,
             user=user,
             knowledge_base_id=knowledge_base_id,
-            name=title,
+            name=filename,
             source_type="text",
             content=doc_content,
+            file_extension=".md",
             trigger_indexing=trigger_indexing,
             trigger_summary=trigger_summary,
         )

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/app/mcp_server/tools/dingtalk_docs.py` around lines 195 - 217, The
created document currently uses name=title instead of the timestamped filename
produced by dingtalk_docs_service.build_filename; update the call to
knowledge_orchestrator.create_document_with_content to pass the generated
filename (variable filename) as the name argument (instead of title) so the
stored document uses the `{title}_{timestamp}.md` name returned in the response;
ensure you reference the existing variables filename and title and the function
knowledge_orchestrator.create_document_with_content when making this change.

backend/app/services/dingtalk/docs_service.py (1)

364-369: The export_format parameter is unused.

The export_format parameter is documented and accepted but never used in the implementation. The actual download format is determined by the document's contentType and extension from the MCP response. Consider either removing the unused parameter or implementing format-based export logic if needed.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/app/services/dingtalk/docs_service.py` around lines 364 - 369, The
download_document_content function currently accepts an export_format parameter
but never uses it; either remove the unused parameter or implement logic to
respect it: update download_document_content to inspect export_format and map it
to the appropriate request/conversion path (e.g., force markdown/pdf/html export
when export_format == "markdown"/"pdf"/"html"), or if conversion isn't
supported, remove export_format from the signature and all callers; reference
the download_document_content function and its export_format parameter when
making the change and ensure any tests/call sites are updated accordingly.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

backend/app/mcp_server/tools/dingtalk_docs.py (1)

195-217: ⚠️ Potential issue | 🟠 Major

The generated {title}_{timestamp}.md name is still never applied.

filename is built and returned, but create_document_with_content() still receives name=title. The text-import path therefore does not actually persist the naming convention this tool advertises.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/app/mcp_server/tools/dingtalk_docs.py` around lines 195 - 217, The
code builds a filename via dingtalk_docs_service.build_filename (assigned to
filename) but still passes the original title into
knowledge_orchestrator.create_document_with_content (name=title), so the
generated `{title}_{timestamp}.md` never gets persisted; update the call to
create_document_with_content to pass name=filename (and any other
filename-related parameter the function supports) instead of title so the stored
document uses the built filename from filename.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

backend/app/mcp_server/tools/dingtalk_docs.py (2)

203-204: ⚠️ Potential issue | 🟠 Major

Timestamped filename is computed but not used in persistence path.

Line 204 builds {title}_{timestamp}.md, but Line 227 still passes name=title, so the actual stored/uploaded name does not follow the advertised convention.

Suggested fix

         # Build filename according to naming convention
         filename = dingtalk_docs_service.build_filename(title, modified_time)
+        name_without_ext, ext = filename.rsplit(".", 1)

@@
         result = await asyncio.to_thread(
             knowledge_orchestrator.create_document_with_content,
             db=db,
             user=user,
             knowledge_base_id=knowledge_base_id,
-            name=title,
+            name=name_without_ext,
             source_type="text",
             content=doc_content,
-            file_extension=file_extension,
+            file_extension=ext,
             trigger_indexing=trigger_indexing,
             trigger_summary=trigger_summary,
             source_config=source_config,
         )

Also applies to: 222-234

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/app/mcp_server/tools/dingtalk_docs.py` around lines 203 - 204, The
timestamped filename created by dingtalk_docs_service.build_filename(title,
modified_time) is computed into the variable filename but never used; update the
persistence/upload call that currently passes name=title to instead pass
name=filename (i.e., use the filename variable), ensuring the stored/uploaded
document follows the {title}_{timestamp}.md convention; look for usages around
the call that sets filename and the subsequent persist/upload function where
name=title and replace that argument.

---

33-41: ⚠️ Potential issue | 🟠 Major

Trace decorators are still missing on all newly added MCP handlers.

get_dingtalk_document_info, add_dingtalk_doc_to_knowledge, and add_dingtalk_doc_with_attachment should be wrapped with @trace_async / @trace_sync to make this flow observable.

As per coding guidelines, "Use @trace_async(span_name, tracer_name, extract_attributes) decorator to trace entire async functions in OpenTelemetry" and "Use @trace_sync(span_name, tracer_name, extract_attributes) decorator to trace entire sync functions in OpenTelemetry".

Also applies to: 90-104, 254-266

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/app/mcp_server/tools/dingtalk_docs.py` around lines 33 - 41, The
three newly added MCP handler functions get_dingtalk_document_info,
add_dingtalk_doc_to_knowledge, and add_dingtalk_doc_with_attachment need
OpenTelemetry decorators; wrap each async handler with `@trace_async`(span_name,
tracer_name, extract_attributes) and any sync handler with `@trace_sync`(...) so
their execution is observable: add `@trace_async` for get_dingtalk_document_info
and for any other async functions (e.g., add_dingtalk_doc_with_attachment if
async), and use `@trace_sync` for synchronous handlers (e.g.,
add_dingtalk_doc_to_knowledge if sync), providing a clear span_name (e.g.,
"mcp.get_dingtalk_document_info"), the appropriate tracer_name, and an
extract_attributes lambda that pulls relevant params (like doc_url, node_id) to
ensure consistent tracing across these handlers.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

backend/app/mcp_server/tools/dingtalk_docs.py (3)

140-142: ⚠️ Potential issue | 🔴 Critical

Do not pass db/user across asyncio.to_thread.

db (SQLAlchemy session) and user (ORM object) are created on the main thread and then used in a worker thread. This is thread-unsafe and can cause runtime/data consistency failures.

Proposed fix (keep blocking work off-loop, but create thread-local DB/ORM objects)

@@
-        result = await asyncio.to_thread(
-            knowledge_orchestrator.create_document_with_content,
-            db=db,
-            user=user,
-            knowledge_base_id=knowledge_base_id,
-            name=title,
-            source_type="text",
-            content=doc_content,
-            file_extension=file_extension,
-            trigger_indexing=trigger_indexing,
-            trigger_summary=trigger_summary,
-            source_config=source_config,
-        )
+        def _create_document_in_thread():
+            thread_db = SessionLocal()
+            try:
+                thread_user = _get_user_from_token(thread_db, token_info)
+                if not thread_user:
+                    raise ValueError("User not found")
+                return knowledge_orchestrator.create_document_with_content(
+                    db=thread_db,
+                    user=thread_user,
+                    knowledge_base_id=knowledge_base_id,
+                    name=title,
+                    source_type="text",
+                    content=doc_content,
+                    file_extension=file_extension,
+                    trigger_indexing=trigger_indexing,
+                    trigger_summary=trigger_summary,
+                    source_config=source_config,
+                )
+            finally:
+                thread_db.close()
+
+        result = await asyncio.to_thread(_create_document_in_thread)

#!/bin/bash
set -euo pipefail

echo "Check to_thread callsite and passed args:"
rg -n -C4 'asyncio\.to_thread\(' backend/app/mcp_server/tools/dingtalk_docs.py

echo
echo "Check orchestrator signature and DB/ORM expectations:"
rg -n -C10 'def create_document_with_content\(' backend/app/services/knowledge/orchestrator.py

Also applies to: 207-219

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/app/mcp_server/tools/dingtalk_docs.py` around lines 140 - 142, You
are passing a SessionLocal() instance and an ORM user object from the main
thread into asyncio.to_thread, which is thread-unsafe; instead, change the
to_thread callsite so it does not receive db or ORM objects—pass only primitives
(e.g., user id or token_info) and move SessionLocal() creation and the ORM query
(the equivalent of _get_user_from_token) into the worker thread (or open a new
session inside create_document_with_content / the threaded helper). Locate the
asyncio.to_thread usage in dingtalk_docs.py and ensure the worker function
creates its own SessionLocal(), re-queries the User by id (or re-validates
token), and closes the session; apply the same fix pattern to the similar
callsite around the create_document_with_content orchestration mentioned in the
comment.

---

302-307: ⚠️ Potential issue | 🟡 Minor

Return the created document’s attachment id, not the input id.

The response currently echoes the input attachment_id; this can become inaccurate if the create flow remaps/copies attachments.

Proposed fix

         return {
             "success": True,
             "document_id": result.id,
             "document_name": result.name,
-            "attachment_id": attachment_id,
+            "attachment_id": getattr(result, "attachment_id", attachment_id),
             "message": f"Document '{doc_title}' added to knowledge base successfully",
         }

#!/bin/bash
set -euo pipefail

echo "Inspect attachment handling in orchestrator:"
rg -n -C8 'attachment_id|source_type.*attachment|copy' backend/app/services/knowledge/orchestrator.py

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/app/mcp_server/tools/dingtalk_docs.py` around lines 302 - 307, The
response is echoing the input attachment_id instead of the attachment id
returned by the create flow; update the return dict in
backend/app/mcp_server/tools/dingtalk_docs.py to use the created attachment id
from the API result (e.g., use result.attachment_id or result.attachment.id or
result.attachments[0].id as provided by the create call) instead of the input
variable attachment_id, and add a safe fallback (None) if that field is missing
so the returned "attachment_id" reflects the actual stored attachment; adjust
tests or callers if they expect the old input value.

---

33-41: ⚠️ Potential issue | 🟠 Major

Add tracing decorators to all three MCP handlers.

These entrypoints are untraced, so the DingTalk import path is not visible in OpenTelemetry.

As per coding guidelines, "Use @trace_async(span_name, tracer_name, extract_attributes) decorator to trace entire async functions in OpenTelemetry" and "Use @trace_sync(span_name, tracer_name, extract_attributes) decorator to trace entire sync functions in OpenTelemetry".

Also applies to: 90-104, 239-251

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/app/mcp_server/tools/dingtalk_docs.py` around lines 33 - 41, Add
OpenTelemetry tracing decorators to the three MCP handler entrypoints in this
file: attach `@trace_async` to get_dingtalk_document_info and the two other
handler functions defined around the ranges referenced (lines ~90-104 and
~239-251); for each async handler use `@trace_async` with span_name set to a
descriptive value (e.g., "mcp.get_dingtalk_document_info" for
get_dingtalk_document_info), tracer_name "mcp", and an extract_attributes
callable that extracts the doc_url (or other relevant params) into attributes so
the DingTalk import path and request context are visible in traces; if any
handler is sync use `@trace_sync` with the same naming/attributes convention.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

backend/init_data/skills/dingtalk-connector/provider.py (2)

598-614: ⚠️ Potential issue | 🟠 Major

Handle /wiki/ URLs in the download step too.

_get_document_info_from_mcp() already accepts DingTalk wiki links, but _download_document_content_real() only extracts IDs from /nodes/ and /docs/. A valid wiki URL will therefore pass step 2 and fail immediately at step 3 with “Could not extract document ID”.

Minimal fix

             patterns = [
                 r"alidocs\.dingtalk\.com/i/nodes/([a-zA-Z0-9_-]+)",
                 r"alidocs\.dingtalk\.com/i/team/[^/]+/docs/([a-zA-Z0-9_-]+)",
+                r"alidocs\.dingtalk\.com/i/team/[^/]+/wiki/([a-zA-Z0-9_-]+)",
             ]

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/init_data/skills/dingtalk-connector/provider.py` around lines 598 -
614, _download_document_content_real() fails to extract IDs for DingTalk wiki
links even though _get_document_info_from_mcp() accepts them; update the
ID-extraction logic in _download_document_content_real() to include the wiki URL
pattern used elsewhere (e.g., add a regex covering "/wiki/" URLs similar to the
patterns for "/i/nodes/" and "/i/team/.../docs/") so that doc_id is correctly
set for wiki links and the function no longer returns the "Could not extract
document ID" error.

---

631-644: ⚠️ Potential issue | 🔴 Critical

download_document is not a registered MCP tool.

The knowledge server registers the decorated tools from backend/app/mcp_server/tools/dingtalk_docs.py, and that file exposes get_dingtalk_document_info, add_dingtalk_doc_to_knowledge, and add_dingtalk_doc_with_attachment — not download_document. This request will fail as soon as the content-download step runs.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/init_data/skills/dingtalk-connector/provider.py` around lines 631 -
644, The payload is calling an unregistered MCP tool "download_document";
replace it with a registered tool (e.g., "add_dingtalk_doc_with_attachment" or
"get_dingtalk_document_info") or register a new tool. Update the "name" field in
the payload from "download_document" to one of the existing exported tools
(suggest "add_dingtalk_doc_with_attachment" if you intend to upload the
downloaded content into knowledge) and ensure the arguments match that tool's
signature (use doc_id, url and any expected keys), or alternatively
implement/register a new tool named "download_document" in
backend/app/mcp_server/tools/dingtalk_docs.py if that behavior is required.

backend/app/services/dingtalk/docs_service.py (2)

158-172: ⚠️ Potential issue | 🟡 Minor

Chain the original HTTPStatusError when re-raising.

Both except httpx.HTTPStatusError as e blocks discard the original traceback by raising a fresh ValueError without from e, which makes MCP/download failures much harder to debug.

Also applies to: 414-427

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/app/services/dingtalk/docs_service.py` around lines 158 - 172, The
except blocks catching httpx.HTTPStatusError (e) in docs_service.py currently
re-raise new ValueError instances and discard the original exception context;
update each re-raise in the HTTPStatusError handlers (the block shown and the
similar block around lines 414-427) to chain the original exception by using
"raise ValueError(... ) from e" so the original HTTPStatusError traceback is
preserved; ensure you apply this to all branches (AUTH_ERROR_CODES branch, 406
branch, and the generic failure branch) that construct and raise ValueError.

---

272-277: ⚠️ Potential issue | 🟡 Minor

Reject or honor unsupported export_format values.

export_format is part of the public API here, but the method never validates it or passes it downstream. A caller asking for html or txt currently gets whatever the MCP returns, while file_extension is still inferred from the DingTalk metadata instead of the requested format.

Also applies to: 339-412

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/app/services/dingtalk/docs_service.py` around lines 272 - 277,
download_document_content currently accepts an export_format but never validates
or enforces it; update it (and the comparable methods in the 339-412 range) to
validate export_format against an allowed set (e.g.,
{"markdown","html","txt","pdf"} or the formats supported by the MCP), raise a
clear exception for unsupported values, and ensure the validated format is
propagated to downstream requests and used to derive file_extension (instead of
relying on DingTalk metadata); reference the download_document_content function
name and the downstream call(s) that send export_format/file_extension so you
can locate where to inject validation, mapping, and the error raise.

backend/app/mcp_server/tools/dingtalk_docs.py (2)

300-305: ⚠️ Potential issue | 🟡 Minor

Return the attachment actually linked to the new document.

Echoing the input attachment_id here can be stale if the orchestrator copies or rebinds the attachment during document creation. Return result.attachment_id so callers see the attachment that the new knowledge document actually references.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/app/mcp_server/tools/dingtalk_docs.py` around lines 300 - 305, The
returned payload currently echoes the input attachment_id which can be stale;
update the return dict in the function that builds the response (the block using
variables result, doc_title, attachment_id) to return the attachment actually
linked to the created document by using result.attachment_id for the
"attachment_id" field instead of the input attachment_id so callers receive the
real attachment bound to the new document.

---

205-217: ⚠️ Potential issue | 🔴 Critical

Keep the SQLAlchemy session and ORM object on the same thread.

db is created at Line 136 and user is loaded from it before the asyncio.to_thread() call, but both are then used inside the worker thread. SQLAlchemy sessions and ORM instances are not thread-safe, so this can break once create_document_with_content() touches the session in that background thread.

Safer pattern

-        result = await asyncio.to_thread(
-            knowledge_orchestrator.create_document_with_content,
-            db=db,
-            user=user,
-            knowledge_base_id=knowledge_base_id,
-            name=title,
-            source_type="text",
-            content=doc_content,
-            file_extension=file_extension,
-            trigger_indexing=trigger_indexing,
-            trigger_summary=trigger_summary,
-            source_config=source_config,
-        )
+        def _create_document() -> Any:
+            thread_db = SessionLocal()
+            try:
+                thread_user = _get_user_from_token(thread_db, token_info)
+                if not thread_user:
+                    raise ValueError("User not found")
+                return knowledge_orchestrator.create_document_with_content(
+                    db=thread_db,
+                    user=thread_user,
+                    knowledge_base_id=knowledge_base_id,
+                    name=title,
+                    source_type="text",
+                    content=doc_content,
+                    file_extension=file_extension,
+                    trigger_indexing=trigger_indexing,
+                    trigger_summary=trigger_summary,
+                    source_config=source_config,
+                )
+            finally:
+                thread_db.close()
+
+        result = await asyncio.to_thread(_create_document)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/app/mcp_server/tools/dingtalk_docs.py` around lines 205 - 217, The
code passes a SQLAlchemy Session object `db` and an ORM `user` into
`asyncio.to_thread()` where
`knowledge_orchestrator.create_document_with_content` runs; SQLAlchemy
sessions/ORM instances are not thread-safe—either perform all DB/ORM work on the
current async thread (call `create_document_with_content` directly without
`asyncio.to_thread`) or pass only primitive data into the worker and open a new
Session there. Concretely, extract any IDs/primitive fields needed from `user`
(e.g., `user.id`) and other DB-derived values before calling
`asyncio.to_thread`, or move the call out of `asyncio.to_thread` and `await
knowledge_orchestrator.create_document_with_content(db=..., user=...)` on the
original thread; alternatively, modify `create_document_with_content` to accept
primitives and create its own session inside the worker thread.

backend/app/services/dingtalk/docs_service.py (1)

207-211: Trace the new async service entrypoints.

get_document_info() and download_document_content() are the backend entrypoints for this feature, but neither emits a span today, so DingTalk fetch/download failures will be missing from the trace tree.

As per coding guidelines, "Use @trace_async(span_name, tracer_name, extract_attributes) decorator to trace entire async functions in OpenTelemetry".

Also applies to: 272-277

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/app/services/dingtalk/docs_service.py` around lines 207 - 211, Add
OpenTelemetry tracing to the async entrypoints by importing and applying the
trace_async decorator: add `@trace_async`("dingtalk.get_document_info",
"dingtalk", extract_attributes=...) above the async def get_document_info(...)
and add `@trace_async`("dingtalk.download_document_content", "dingtalk",
extract_attributes=...) above async def download_document_content(...); ensure
trace_async is imported from the tracing/observability helper used elsewhere in
the repo and have the extract_attributes include relevant params (e.g., doc_url
and user_preferences for get_document_info, doc_url for
download_document_content) so failures appear in the trace tree.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

backend/init_data/skills/dingtalk-connector/provider.py (3)

62-79: Use raise ... from err for better exception chaining.

When re-raising an exception within an except block, use raise ... from err (or raise ... from None) to preserve the exception chain and distinguish the new exception from errors in exception handling.

♻️ Suggested fix

 except ImportError:
     # Try absolute import (for dynamic loading)
     import sys
 
     # Get the package name dynamically
     package_name = __name__.rsplit(".", 1)[0]
     _base_module = sys.modules.get(f"{package_name}._base")
     if _base_module:
         BaseSandboxTool = _base_module.BaseSandboxTool
     else:
-        raise ImportError(
+        raise ImportError(
             "Cannot import BaseSandboxTool from chat_shell.tools.sandbox._base"
-        )
+        ) from None

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/init_data/skills/dingtalk-connector/provider.py` around lines 62 -
79, The ImportError re-raise in the fallback import path should chain the
original exception; in the except ImportError block that constructs package_name
and looks up _base_module (and ultimately raises ImportError("Cannot import
BaseSandboxTool from chat_shell.tools.sandbox._base")), change the re-raise to
preserve the original error by using "raise ImportError(... ) from err" (or from
the caught exception variable) so the original ImportError is chained to the new
one when BaseSandboxTool cannot be resolved; locate the try/except around the
import of BaseSandboxTool and update the final raise to use exception chaining.

---

554-598: Remove or deprecate dead placeholder code.

_download_document_content() is never called — the workflow uses _download_document_content_real() instead. Keeping this placeholder creates confusion about which method is the actual implementation.

♻️ Suggested action

Either remove _download_document_content entirely, or rename _download_document_content_real to _download_document_content and delete the placeholder.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/init_data/skills/dingtalk-connector/provider.py` around lines 554 -
598, The placeholder method _download_document_content is dead code and
conflicts with the real implementation _download_document_content_real; either
remove _download_document_content entirely, or move/rename the real
implementation by renaming _download_document_content_real to
_download_document_content and delete the placeholder, then update any internal
references to use the canonical name so only the real implementation remains;
ensure imports/uses refer to the chosen name and run tests to confirm no
references remain to the removed function.

---

540-550: Add logging to silent exception handlers.

Multiple try-except-pass blocks silently swallow exceptions during temp file cleanup (lines 548-549, 728-729, 897-898, 908-909). While cleanup failures may be non-critical, logging at DEBUG level helps diagnose sandbox issues.

♻️ Example fix for one instance

             finally:
                 # Clean up temp file
                 try:
                     await sandbox.commands.run(
                         cmd=f"rm -f {payload_file}",
                         cwd="/home/user",
                         timeout=10,
                     )
-                except Exception:
-                    pass
+                except Exception as cleanup_err:
+                    logger.debug(f"[DingTalkDocToKBTool] Temp file cleanup failed: {cleanup_err}")

Also applies to: 720-730, 890-910

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/init_data/skills/dingtalk-connector/provider.py` around lines 540 -
550, The cleanup code currently swallows exceptions in multiple
finally/except-pass blocks around sandbox.commands.run (e.g., the rm -f
{payload_file} call and similar temp-file cleanup calls), so add logging at
DEBUG level in those except handlers: create/get a module logger (e.g., logger =
logging.getLogger(__name__)) and replace the bare "except Exception: pass" with
a handler that logs a clear message including the resource name (payload_file or
other temp filename) and the caught exception (use exc_info=True or include
traceback). Apply the same change to all similar blocks that silently pass (the
cleanup calls around sandbox.commands.run and temp file removals referenced in
the review).

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

backend/init_data/skills/dingtalk-connector/provider.py (3)

62-79: Use raise ... from None for cleaner exception chaining.

When raising a new ImportError after failing to find the module dynamically, use raise ... from None to suppress the confusing chained exception context.

♻️ Suggested fix

     if _base_module:
         BaseSandboxTool = _base_module.BaseSandboxTool
     else:
-        raise ImportError(
+        raise ImportError(
             "Cannot import BaseSandboxTool from chat_shell.tools.sandbox._base"
-        )
+        ) from None

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/init_data/skills/dingtalk-connector/provider.py` around lines 62 -
79, The ImportError raised when _base_module is not found should suppress
chained exception context; update the else branch where you raise
ImportError("Cannot import BaseSandboxTool from chat_shell.tools.sandbox._base")
to use "raise ImportError(... ) from None" so the new ImportError is raised
without showing the previous exception context—make this change in the block
that resolves BaseSandboxTool (variables: package_name, _base_module,
BaseSandboxTool).

---

554-598: Remove unused placeholder method.

_download_document_content returns hardcoded placeholder content and is not called anywhere—the actual implementation is _download_document_content_real (used at line 309). Remove this dead code to avoid confusion.

🗑️ Remove dead code

-    async def _download_document_content(self, sandbox: Any, doc_url: str) -> dict:
-        """Download document content from DingTalk.
-
-        For now, this returns placeholder content.
-        In production, this would call the DingTalk API via MCP.
-        """
-        try:
-            # Extract doc ID for content generation
-            import re
-
-            doc_id = None
-            patterns = [
-                r"alidocs\.dingtalk\.com/i/nodes/([a-zA-Z0-9_-]+)",
-                r"alidocs\.dingtalk\.com/i/team/[^/]+/docs/([a-zA-Z0-9_-]+)",
-            ]
-
-            for pattern in patterns:
-                match = re.search(pattern, doc_url)
-                if match:
-                    doc_id = match.group(1)
-                    break
-
-            # Generate placeholder content
-            content = f"""# DingTalk Document
-
-This document was imported from DingTalk.
-
-**Source URL:** {doc_url}
-**Document ID:** {doc_id or "unknown"}
-**Imported at:** {datetime.now().isoformat()}
-
-## Content
-
-The actual content would be fetched from DingTalk API in production.
-This is a placeholder for the document content.
-
----
-*Imported by Wegent DingTalk Docs Skill*
-"""
-
-            return {"success": True, "content": content}
-
-        except Exception as e:
-            return {"success": False, "error": str(e)}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/init_data/skills/dingtalk-connector/provider.py` around lines 554 -
598, Remove the unused placeholder function _download_document_content from the
DingTalk provider (it returns hardcoded content and is not referenced); locate
and delete the async def _download_document_content(self, sandbox: Any, doc_url:
str) block, ensuring only the real implementation
_download_document_content_real remains and that no other code references the
removed symbol (search for _download_document_content to confirm).

---

418-553: Extract duplicated URL parsing logic into a helper.

The URL pattern matching to extract doc_id is duplicated in _get_document_info_from_mcp (lines 428-438), _download_document_content (lines 565-574), and _download_document_content_real (lines 609-619). Extract this into a private helper method to reduce duplication and ensure consistent pattern support.

♻️ Example helper extraction

def _extract_doc_id_from_url(self, doc_url: str) -> Optional[str]:
    """Extract document ID from DingTalk URL.
    
    Supports nodes, docs, and wiki URL formats.
    """
    import re
    
    patterns = [
        r"alidocs\.dingtalk\.com/i/nodes/([a-zA-Z0-9_-]+)",
        r"alidocs\.dingtalk\.com/i/team/[^/]+/docs/([a-zA-Z0-9_-]+)",
        r"alidocs\.dingtalk\.com/i/team/[^/]+/wiki/([a-zA-Z0-9_-]+)",
    ]
    
    for pattern in patterns:
        match = re.search(pattern, doc_url)
        if match:
            return match.group(1)
    return None

Then replace the duplicated pattern matching in each method with:

doc_id = self._extract_doc_id_from_url(doc_url)
if not doc_id:
    return {"success": False, "error": f"Could not extract document ID from URL: {doc_url}"}

Also applies to: 599-733

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/init_data/skills/dingtalk-connector/provider.py` around lines 418 -
553, The URL extraction regex is duplicated across _get_document_info_from_mcp,
_download_document_content, and _download_document_content_real; add a private
helper method (e.g., _extract_doc_id_from_url(self, doc_url: str) ->
Optional[str]) that contains the three regex patterns and returns the matched
group or None, then replace each local regex block with a call to this helper
and preserve the existing error handling (returning {"success": False, "error":
...} when None); update the three methods to use the helper to ensure consistent
behavior and remove the duplicated imports/usages of re and repeated pattern
lists.

backend/app/mcp_server/tools/dingtalk_docs.py (1)

200-212: Move imports to module level.

_build_filename (line 201) and asyncio (line 212) are imported inside the function. Move these to the top of the file for consistency and to avoid repeated import overhead.

Additionally, _build_filename is a private function (underscore prefix). Consider whether this should be exposed as a public API or if the filename building logic should be duplicated here to avoid depending on private internals that may change.

♻️ Suggested refactor

 import logging
+import asyncio
 from typing import Any, Dict, Optional
 
 from app.db.session import SessionLocal
 from app.mcp_server.auth import TaskTokenInfo
 from app.mcp_server.tools.decorator import build_mcp_tools_dict, mcp_tool
 from app.mcp_server.tools.knowledge import _get_user_from_token
 from app.services.dingtalk.docs_service import dingtalk_docs_service
-from app.services.knowledge.orchestrator import knowledge_orchestrator
+from app.services.knowledge.orchestrator import knowledge_orchestrator, _build_filename
 from shared.telemetry.decorators import trace_async, trace_sync

Then remove the inline imports at lines 201 and 212.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/app/mcp_server/tools/dingtalk_docs.py` around lines 200 - 212, Move
the inline imports to module level: add "from
app.services.knowledge.orchestrator import _build_filename" and "import asyncio"
at the top of the module and remove the in-function imports around the calls to
_build_filename and asyncio; also evaluate whether to replace usage of the
private helper _build_filename (underscore-prefixed) by promoting it to a public
function (rename/export) or by duplicating the filename-building logic locally
so the module does not depend on a private internal API.

backend/app/mcp_server/tools/dingtalk_docs.py (1)

395-400: ⚠️ Potential issue | 🟡 Minor

Return the actual linked attachment id from the created document.

This response echoes the input attachment_id, which may differ from the attachment actually linked by orchestrator.

Suggested fix

         return {
             "success": True,
             "document_id": result.id,
             "document_name": result.name,
-            "attachment_id": attachment_id,
+            "attachment_id": (
+                result.attachment_id
+                if getattr(result, "attachment_id", None) is not None
+                else attachment_id
+            ),
             "message": f"Document '{doc_title}' added to knowledge base successfully",
         }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/app/mcp_server/tools/dingtalk_docs.py` around lines 395 - 400, The
return currently echoes the input attachment_id rather than the actual
attachment linked by the orchestrator; update the return to read the linked
attachment id from the created document object (the result variable) — e.g. use
result.attachment_id or result.linked_attachment.id or
result.linked_attachment_id (whichever exists on the returned object) and fall
back to the original attachment_id if none is present, then return that value in
the "attachment_id" field along with document_id/result.id and
document_name/result.name.

backend/app/mcp_server/tools/dingtalk_docs.py (1)

167-189: Narrow the inner exception handling around DingTalk fetch.

Catching Exception here masks unexpected programming/runtime errors and duplicates outer error handling. Prefer catching expected fetch failures only (e.g., ValueError) and let unknown exceptions bubble to the outer except with stack trace.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/app/mcp_server/tools/dingtalk_docs.py` around lines 167 - 189, The
current broad except in the block calling
dingtalk_docs_service.download_document_content masks unexpected errors; narrow
the handler to only expected fetch failures (e.g., ValueError or a specific
service fetch exception) around the call to
dingtalk_docs_service.download_document_content and related parsing
(doc_download, doc_content, file_extension, modified_time, update_time), log and
return the graceful error only for those expected exceptions, and let other
exceptions propagate (or re-raise) so the outer exception handler can capture
stack traces.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes